Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sessionctx, executor: add session var to control explicit insertion on auto_random column #17102

Merged
merged 6 commits into from
May 26, 2020
Merged

sessionctx, executor: add session var to control explicit insertion on auto_random column #17102

merged 6 commits into from
May 26, 2020

Conversation

tangenta
Copy link
Contributor

@tangenta tangenta commented May 11, 2020 •
edited
Loading

What problem does this PR solve?

Problem Summary:

As the documentation says,

It is NOT recommended that you explicitly specify a value for the column with the AUTO_RANDOM attribute when you insert data. Otherwise, the numeral values that can be automatically assigned for this table might be used up in advance.

This PR adds a session variable named tidb_allow_auto_random_explicit_insert, which controls whether explicit insertion on auto_random column is allowed. By default, it is false.

What is changed and how it works?

What's Changed:

  • Add a new session variable tidb_allow_auto_random_explicit_insert.
  • Add a few tests.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Breaking backward compatibility (Fortunately, auto_random is under experimental.)

Release note

  • a new session variable named tidb_allow_auto_random_explicit_insert is added, which controls whether explicit insertion on auto_random column is allowed.

@tangenta tangenta requested a review from a team as a code owner May 11, 2020 11:21
@ghost ghost requested review from wshwsh12 and removed request for a team May 11, 2020 11:21
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label May 11, 2020
@codecov
Copy link

codecov bot commented May 12, 2020 •
edited
Loading

Codecov Report

Merging #17102 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #17102   +/-   ##
===========================================
  Coverage   79.9121%   79.9121%           
===========================================
  Files           520        520           
  Lines        140269     140269           
===========================================
  Hits         112092     112092           
  Misses        19217      19217           
  Partials       8960       8960

djshow832
djshow832 reviewed May 12, 2020
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The title and PR description are all about implicit insertion but the code is all explicit insertion. Please modify the PR title and description.

@tangenta tangenta changed the title sessionctx, executor: add session var to control implicit insertion on auto_random column sessionctx, executor: add session var to control explicit insertion on auto_random column May 12, 2020
djshow832
djshow832 reviewed May 12, 2020
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why session variable, not global variable?

@tangenta
Copy link
Contributor Author

Why session variable, not global variable?

@djshow832 Although I think the case is rare, imagine there are two sessions:
session 1: importing data from the migration tool to table t1
session 2: inserting data from users to table t2, which needs the protection provided by this variable.

Global variable cannot meet the demand.

djshow832
djshow832 reviewed May 26, 2020
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

meta/autoid/errors.go Outdated Show resolved Hide resolved
crazycs520
crazycs520 previously approved these changes May 26, 2020
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crazycs520 crazycs520 dismissed their stale review May 26, 2020 08:19

need 1 lgtm

djshow832
djshow832 approved these changes May 26, 2020
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tangenta
Copy link
Contributor Author

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 26, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 26, 2020

/run-all-tests

@sre-bot sre-bot merged commit a3d5082 into pingcap:master May 26, 2020
@tangenta
Copy link
Contributor Author

/run-cherry-picker

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request May 27, 2020
@tangenta @sre-bot
cherry pick pingcap#17102 to release-3.1
209b0ad 
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot sre-bot mentioned this pull request May 27, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented May 27, 2020

cherry pick to release-3.1 in PR #17436

zimulala pushed a commit that referenced this pull request May 28, 2020
@sre-bot
sessionctx, executor: add session var to control explicit insertion o…
34367b6 
…n auto_random column (#17102) (#17436)
july2993 added a commit to july2993/tidb-binlog that referenced this pull request May 31, 2020
@july2993
Enable allow_auto_random_explicit_insert if it's suppported
859a02b 
Added by pingcap/tidb#17102
default is false, must enable for insert value explicit, or can't
replicate.
@july2993 july2993 mentioned this pull request May 31, 2020
Merged
july2993 added a commit to pingcap/tidb-binlog that referenced this pull request Jun 1, 2020
@july2993
Enable allow_auto_random_explicit_insert if it's suppported (#975)
c4a4e51 
* Enable allow_auto_random_explicit_insert if it's suppported

Added by pingcap/tidb#17102
default is false, must enable for insert value explicit, or can't
replicate.
july2993 added a commit to july2993/tidb-binlog that referenced this pull request Jun 1, 2020
@july2993
Enable allow_auto_random_explicit_insert if it's suppported (pingcap#975
e9a851f 
)

* Enable allow_auto_random_explicit_insert if it's suppported

Added by pingcap/tidb#17102
default is false, must enable for insert value explicit, or can't
replicate.
@july2993 july2993 mentioned this pull request Jun 1, 2020
Merged
july2993 added a commit to pingcap/tidb-binlog that referenced this pull request Jun 1, 2020
@july2993
Enable allow_auto_random_explicit_insert if it's suppported (#975) (#976
cde1d7f 
)

* Enable allow_auto_random_explicit_insert if it's suppported

Added by pingcap/tidb#17102
default is false, must enable for insert value explicit, or can't
replicate.
tangenta added a commit to tangenta/tidb that referenced this pull request Jul 13, 2020
@tangenta
sessionctx, executor: add session var to control explicit insertion o…
617b8d2 
…n auto_random column (pingcap#17102)
@tangenta tangenta mentioned this pull request Jul 13, 2020
Merged
bb7133 pushed a commit that referenced this pull request Jul 13, 2020
@tangenta
sessionctx, executor: add session var to control explicit insertion o…
89e34b7 
…n auto_random column (#17102) (#18508)
@kolbe kolbe mentioned this pull request Jul 31, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djshow832 djshow832 djshow832 approved these changes

@crazycs520 crazycs520 crazycs520 left review comments

@wshwsh12 wshwsh12 Awaiting requested review from wshwsh12

Assignees
No one assigned
Labels
component/session sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tangenta @sre-bot @crazycs520 @djshow832